Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support build against LibreSSL #1451

Closed
wants to merge 5 commits into from
Closed

Conversation

marcfir
Copy link

@marcfir marcfir commented Feb 16, 2024

Some projects use LibreSSL instead of OpenSSL.
In umati/Dashboard-OPCUA-Client we use a patch for this, but direct support would simplify our build https://github.com/umati/Dashboard-OPCUA-Client#633.

This also solves #1426

@GoetzGoerisch
Copy link

@icraggs anything we can help to get this merged?

@icraggs
Copy link
Contributor

icraggs commented Mar 5, 2024

My main question here is how does this get tested? What OSes would be supported? At the moment the CI tests are run for MacOS, Linux and Windows. Installation of LibreSSL in a Linux environment, at least on my usual Ubuntu OS, seems like it's not a readily installable package and it's not available by default in the CI environments I've looked at so far. I could merge it as unsupported but that would not be ideal.

@GoetzGoerisch
Copy link

Thanks for the explanation.
So adding a workflow which builds and tests the compile option would be sufficient?

@icraggs
Copy link
Contributor

icraggs commented Mar 5, 2024

That would be a great help, yes. I also have to check the CMake changes against PR #1427 which I haven't merged yet, but intend to do so.

@marcfir
Copy link
Author

marcfir commented Mar 25, 2024

@icraggs I added tests for Linux and MacOS. The tests on Windows go into a timeout CI LibreSSL. But tests with OpenSSL do the same CI OpenSSL.

@GoetzGoerisch
Copy link

@icraggs anything missing to get this merged?

@icraggs
Copy link
Contributor

icraggs commented Apr 16, 2024

@GoetzGoerisch I've been in North America for a week for the eclipse. I'm back now, and just been merging Frank's PR for CMake into the 1.4 branch. This PR is next on my list.

@marcfir marcfir changed the base branch from master to 1.4 April 17, 2024 04:40
@marcfir marcfir changed the base branch from 1.4 to master April 17, 2024 04:40
Copy link
Contributor

@icraggs icraggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 questions as a result of merging

IF (PAHO_WITH_SSL OR PAHO_WITH_LIBRESSL)

IF (PAHO_WITH_LIBRESSL)
SET(OPENSSL_ROOT_DIR "" CACHE PATH "Directory containing LibreSSL libraries and includes")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In merging this PR, I don't understand this line. Are we zeroing out the LIBRESSL root_dir, in which case the first parameter should be LIBRESSL_ROOT_DIR?

SET(SSL_INCLUDE_DIR ${LIBRESSL_INCLUDE_DIR} CACHE PATH "Directory containing SSL includes")
SET(SSL_LIBRARY_NAME LibreSSL CACHE STRING "Name of the used SSL library")
ELSE()
SET(LIBRESSL_ROOT_DIR "" CACHE PATH "Directory containing OpenSSL libraries and includes")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above but in reverse

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these lines are mixed up

SET(SSL_INCLUDE_DIR ${OPENSSL_INCLUDE_DIR} CACHE PATH "Directory containing SSL includes")
SET(SSL_LIBRARY_NAME OpenSSL CACHE STRING "Name of the used SSL library")
ENDIF()
message("Use ${SSL_LIBRARY_NAME} at ${LIBRESSL_ROOT_DIR}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right for OPENSSL, is it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I add a fix for that.

@icraggs icraggs self-assigned this Apr 17, 2024
@marcfir marcfir requested a review from icraggs April 18, 2024 11:09
@icraggs
Copy link
Contributor

icraggs commented Apr 18, 2024

Ok, I merged the PR yesterday, with a couple of changes of my own to answer my questions, into the develop and 1.4 branches. You can check my changes there.

If you propose any further changes they should be targeted at the develop branch, not master, as the CMake build files have changed due to another PR I've merged.

@icraggs
Copy link
Contributor

icraggs commented Apr 18, 2024

I'm going to close this PR now, as it's in the develop/1.4 branches. If you have any further comments you can make them here or in another PR/issue. Thanks for your contributions.

@icraggs icraggs closed this Apr 18, 2024
@fpagliughi
Copy link
Contributor

fpagliughi commented Jul 20, 2024

Apologies, I hadn't seen this PR before it was merged. I just wanted to ask a few questions before this is released. @marcfir

I started writing this message with a number of questions about the use of cache variables where regular variables would have been adequate (preferable?) and why the xxxSSL_ROOT_DIR variables were being cleared each run, as if it were expected that the find_package() call would be setting this...

But then I realized this behavior was already in the CMake files from before the PR!

IF (PAHO_WITH_SSL)
    SET(OPENSSL_ROOT_DIR "" CACHE PATH "Directory containing OpenSSL libraries and includes")
    find_package(OpenSSL REQUIRED)

So I guess the style was just copy-pasted for this PR?

This (the original usage) doesn't seem correct to me. I don't think the CMake file should be randomly clearing a cache variable. The expectation (the purpose of the cache) is to retain variables between runs. Also, the proper usage for OPENSSL/LIBRESSL_ROOT_DIR is for the user to pass in a hint (as an environment variable) about where the library can be found.

Normally this isn't done and the variable is blank, which also makes the current configuration output look broken:

-- Found OpenSSL: /usr/lib/x86_64-linux-gnu/libcrypto.so (found version "3.0.2")  
Use OpenSSL at                                       <--- *** At nowhere? Looks like a bug ***
SSL_INCLUDE_DIR: /usr/include  

Then, when dumping the library information, it would be good to show the include directory and the main SSL library file (with path). That would have helped me recently on Windows where it was finding the headers from and older version (1.1) and then linking to a DLL with a different version (3.x), resulting in missing symbols in the link.

I suggest changing the src/CMakeLists.txt to this:

if(PAHO_WITH_SSL OR PAHO_WITH_LIBRESSL)
  if(PAHO_WITH_LIBRESSL)
    find_package(LibreSSL REQUIRED)
    set(SSL_LIBRARY_NAME LibreSSL)
    message(STATUS "Using LibreSSL with headers at ${LIBRESSL_INCLUDE_DIR}, lib ${LIBRESSL_SSL_LIBRARY}")
  else()
    find_package(OpenSSL REQUIRED)
    set(SSL_LIBRARY_NAME OpenSSL)
    message(STATUS "Using OpenSSL with headers at ${OPENSSL_INCLUDE_DIR}, lib ${OPENSSL_SSL_LIBRARY}")
  endif()
  ...

I can send a PR.

@marcfir
Copy link
Author

marcfir commented Jul 23, 2024

Hi @fpagliughi,
Yes, I just copied the style of including OpenSSL as is. Your suggestion and PR looks very good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants